Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dprintf() polyfill for Illumos #1911

Closed
wants to merge 1 commit into from

Conversation

sjmulder
Copy link
Collaborator

@sjmulder sjmulder commented Jul 15, 2024

I realise it's a bit of a long shot, submitting a PR to fix a legacy platform, but I thought I might at least try upstreaming this patch we've been using for a while.

Edit: this is also for Illumos, which is actively maintained.

@sjmulder sjmulder changed the title dprintf() polyfill for Solaris dprintf() polyfill for Solaris & Illumos Jul 15, 2024
int len, nwritten;

va_start(ap, format);
len = vasprintf(&s, format, ap);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although posix doesn't require it, the general expectation is for dprintf to not do any memory allocation so that it can be used in signal handler context. Not doing allocation would also remove allocation related failure point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be better but I couldn't find a better alternative with the available primitives. One could use a static buffer but then you're length limited.

@@ -121,6 +121,10 @@
#define alloca(size) __builtin_alloca(size)
#endif

#ifdef __sun
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Illumos is maintained, why don't they add dprintf? It's more economical for them to add it instead of N projects to carry their own implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better if they did indeed. But that does leave legacy systems, and even there nnn can be very useful.

@jarun
Copy link
Owner

jarun commented Jul 16, 2024

@N-R-K please merge when ready.

@sjmulder sjmulder changed the title dprintf() polyfill for Solaris & Illumos dprintf() polyfill for Illumos Jul 16, 2024
@sjmulder
Copy link
Collaborator Author

sjmulder commented Jul 16, 2024

Turns out there aren't that many dprintf() calls so perhaps this alternative is better: #1912

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jul 20, 2024
This the improved version submitted to upstream:

jarun/nnn#1911
@jarun
Copy link
Owner

jarun commented Aug 11, 2024

Closed with #1912.

@jarun jarun closed this Aug 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants